-
Notifications
You must be signed in to change notification settings - Fork 839
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Invalid params data #5241
Invalid params data #5241
Conversation
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
|
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to do something similar for web sockets or graphQL? Maybe it'll work for those already, I just noticed only http tests were updated
@@ -41,8 +41,10 @@ public JsonRpcResponse process( | |||
try { | |||
return method.response(request); | |||
} catch (final InvalidJsonRpcParameters e) { | |||
JsonRpcError invalidParamsError = JsonRpcError.INVALID_PARAMS; | |||
invalidParamsError.setData(e.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't touch the Engine API does it?
I think error messages are part of the spec in some cases for that: https://github.com/ethereum/execution-apis/blob/main/src/engine/common.md#errors
On further inspection, calling setData() on an enum value seems like a dodgy hack so I think we need to refactor JsonRpcError to separate these two concepts. Closing this PR, needs further thought. Created an issue #5437 |
Added
data
to error in case of INVALID_PARAMS to give users more info as to what's gone wrongSee #4212 - possible solution to #5098
Not sure if this will break some hive tests?